-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HLM-side changes to allow FATES snow occlusion of LAI #1324
Conversation
…ersus timestepping
Snow occlusion updates fates
FATES developer regression tests ran, expected tests PASS: This PR, coordinated with NGEET/fates#734 should not be b4b when use_fates=true tests, and should be b4b with all others. clm_aux has not been run yet, and the external tag pointer in Externals_CLM will need to be updated once we update the fates tag: https://github.com/ESCOMP/CTSM/blob/master/Externals_CLM.cfg#L5 |
Why is this implemented with an integer rather than a logical? |
It's not completely clear to me what is_called_at_restart means. From the code I think it means if you are restarting and reading the restart file you'd set that to true. But, every other time it would be false. But, the description " ! ifalse = daily timestep, itrue = restart" seems to sound like it refers to when a restart is being written? But, what if restarts aren't written daily? So I think this needs more clarity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked a couple questions in the conversation. The thing that I think needs to happen before bringing this in is a little more clarity on the documentation of what is_called_at_restart is and when it's used.
Thanks @ekluzek.
Good point. I guess when I first wrote it I was thinking there might need to be more than two cases. But it turned out to only need two cases. So I am happy to switch it to logical.
Sure, will do. It is only when a run is being re-initialized from a restart file that this matters. So maybe I will change the variable name to |
Hi @ekluzek I think these changes should address your requested changes? thanks! |
Tested All non-fates testmods PASS b4b. All |
I merged in master (for both fates and ctsm) and re-ran |
All expected
Note that all the fates testmods result in DIFFs, which is as expected. There were two expected non-fates testmod failures. |
This is #3905 so is an expected fail. |
I added this to the expected fails list in a more recent tag, so once you update to the latest master, you should see it noted as "EXPECTED FAILURE". |
We are seeing floating point exception in running longish (6-month) FATES tests now. SMS_D_Lm6.f45_f45_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef I look at the first one and it's a floating point exception at line 1403 in fates/biogeophys/FatesPlantRespPhotosynthMod.F90 which is
So tl must be out of reasonable range. I'm thinking the others are likely for the same reason. @glemieux is going to dig into this further. |
There are some ER tests that are 9 day tests that are failing on izumi. I think they should work if they are changed to 10 day tests. I thought 9 day should work, but looks like it doesn't. So these two tests... ERI_D_Ld9_P48x1.T31_g37.I2000Clm50Sp.izumi_nag.clm-reduceOutput |
I'm confused: haven't these been 9-day tests for a really long time? |
@billsacks I'm confused as well as I know we have tests that use STOP_N==9. There are 9-step tests for example. The error in the TestStatus.log file seems to indicate the length isn't right though...
|
At a glance, it looks to me like these tests were rerun, maybe automatically via the new use of @glemieux can you tell us the history of these tests: in particular, have you already tried rerunning them by hand or not? |
I'll also add: for the sake of making this tag – if this is the last thing holding you up – you could go ahead and create new versions of these failed ERI tests, then we can come back to figuring out what's wrong here. |
Apologies @billsacks and @ekluzek, it looks like I accidentally ran the test suite against the current ctsm master branch instead of the pr branch with master merged in. I just ran one of the 'failing' tests on izumi with the correct PR branch and it passes now. I've got the test suite re-running now. |
All expected tests in the |
@ekluzek do you know when the FATES tests started failing? I.e. is it a change in FATES version or a change in CTSM version that leads to the test failure? |
From what @glemieux told me it works in ctsm5.1.dev030, but now fails in ctsm5.1.dev033. So it looks like it's a CTSM side thing, and Greg thought it due to one of the external updates that came in. I would think a compiler update (if that happened) could be a reason. I wouldn't think that the changes that went into those updates should cause this type of problem, but yet they are, the only answer changing thing had to do with CO2 for historical transient (and really only for coupled to CAM). So it shouldn't be that. I just looked at the cime changes and they seem innocent enough (so no major compiler update for example). |
Misc bfb enhancements and fixes (1) If CISM is running over Antarctica, use virtual glacier columns over Antarctica (2) Remove "mec" from some glacier/ice variable names (it is misleading to have "mec" in variable names when the ice landunit can actually have multiple columns *or* a single column) (ESCOMP#1294) (3) Add history file metadata on each variable's l2g_scale_type (adds a landunit_mask attribute) (ESCOMP#1343) (4) Use python3 in more shebang lines - needed to run python unit tests on cheyenne (5) New compset naming for IG compsets (ESCOMP#1289) (6) Remove calculation of fun_cost_fix that is overwritten (ESCOMP#1115) (7) Bypass grid-level water mass check when fates hydro is active (ESCOMP#1334) (8) Remove some dead code (ESCOMP#1333)
Re-ran |
Re-ran |
FATES hasn't thusfar been letting snow occlude LAI. With PR NGEET/fates#734, it will. This PR accompanies that FATES PR. The reason that CTSM-side changes are needed is that the FATES code needs to handle restarts slightly differently than normal timestepping so as to have bit-for-bit restarts, and so the calling routine needs to tell FATES whether it is calling the snow occlusion code during restarts versus during timestepping.
Description of changes
Just lets FATES know whether it is calling
leaf_area_profile
during restarting versus during timestepping.Specific notes
Accompanies NGEET/fates#734.
Contributors other than yourself, if any:
Discussion with @rgknox @rosiealice @glemieux
CTSM Issues Fixed (include github issue #):
fixes NGEET/fates#359
fixes #1293
Are answers expected to change (and if so in what way)?
When FATES is active, should change answers, particularly in that ELAI is no longer always equal to TLAI. Should not change anything if FATES not active.
Any User Interface Changes (namelist or namelist defaults changes)?
no
Testing performed, if any: standard testing on cheyenne and izumi by @glemieux